fix: separate byte and character limits in BQ plugin GCS text offload#5565
Draft
caohy1988 wants to merge 3 commits intogoogle:mainfrom
Draft
fix: separate byte and character limits in BQ plugin GCS text offload#5565caohy1988 wants to merge 3 commits intogoogle:mainfrom
caohy1988 wants to merge 3 commits intogoogle:mainfrom
Conversation
…plugin When the plugin is deployed via Vertex AI Agent Engine, it is pickled for transport and unpickled on the server. __getstate__ sets _init_pid = 0 as a pickle sentinel. On the server, _ensure_started() checks os.getpid() != self._init_pid, which always evaluates to True since os.getpid() is never 0. This triggers _reset_runtime_state() on every cold start even though no fork happened, producing a misleading "Fork detected (parent PID 0, child PID xx)" warning and adding unnecessary startup latency from tearing down and re-creating gRPC state that was already clear. The fix distinguishes "unpickled, never initialized" (_init_pid == 0) from "forked from a different process" (_init_pid != 0 and _init_pid != os.getpid()). Real forks are still detected by both os.register_at_fork (line 108) and this PID check. Related: haiyuan-eng-google/BigQuery-Agent-Analytics-SDK#86 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After _lazy_setup succeeds, set _init_pid = os.getpid() when it was the pickle sentinel (0). Without this, an unpickled plugin keeps _init_pid == 0 forever, disabling the PID-based fork check for the rest of the instance's lifetime. Also fix test_reset_on_real_fork to use max(os.getpid() - 1, 1) instead of hardcoded 99999. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
adk-bot
added
the
services
Collaborator
|
Response from ADK Triaging Agent Hello @caohy1988, thank you for creating this PR! Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). You can find more information at https://cla.developers.google.com/. Thanks! |
The GCS text offload decision mixed byte-based and character-based limits in a single min() comparison. inline_text_limit (32KB) is a byte-based storage guard, while max_content_length is a character- based truncation limit. Computing min(bytes, chars) produced wrong offload decisions for multi-byte text (CJK, emoji). The fix evaluates each limit in its own unit: - inline_text_limit: compared against UTF-8 byte length - max_content_length: compared against character count Text is offloaded if either limit is exceeded. Includes regression test for the specific google#5561 case: 3K emoji chars (12K bytes) with max_length=10000 — under both real limits but falsely offloaded by the old mixed-unit min(). Fixes google#5561 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
caohy1988
force-pushed
the
fix/bqaa-offload-unit-mismatch
branch
from
7b6e5ef to
040b479
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #5561. Stacked on PR #5528 (fork detection fix).
The GCS text offload decision in
HybridContentParser._parse_content_objectmixed byte-based and character-based limits in a singlemin()comparison, producing wrong offload decisions for multi-byte text.Problem
When
max_content_length < inline_text_limit, the threshold becomes a character count compared against a byte measurement. Example: 3K emoji characters (12K UTF-8 bytes) withmax_length=10000— under both real limits, but the old code computedmin(32768, 10000) = 10000and12K bytes > 10000triggered a false offload.Fix
Evaluate each limit in its own unit — no mixed
min():inline_text_limit(32KB): controls inline storage size — bytesmax_content_length: controls truncation — charactersTest plan
test_multibyte_text_offloaded_by_byte_limit— 10K emoji (40KB UTF-8) offloaded via byte limittest_ascii_under_both_limits_stays_inline— small ASCII stays inlinetest_text_exceeding_char_limit_offloaded— ASCII over char limit but under byte limit is offloadedtest_no_offloader_falls_back_to_truncate— without offloader, truncates inlinetest_multibyte_under_char_and_byte_limits_stays_inline— regression test: 3K emoji (12K bytes) with max_length=10000 stays inline (old code falsely offloaded)🤖 Generated with Claude Code